-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change filter_blank_rows when_any parameter to have a more user-friendly type #7935
Change filter_blank_rows when_any parameter to have a more user-friendly type #7935
Conversation
distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Data/Table.enso
Outdated
Show resolved
Hide resolved
test/Table_Tests/src/Common_Table_Operations/Missing_Values_Spec.enso
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think still a WIP - worth making these in progress PRs draft so we know.
distribution/lib/Standard/Table/0.0.0-dev/src/Data/Blank_Selector.enso
Outdated
Show resolved
Hide resolved
test/Table_Tests/src/Common_Table_Operations/Missing_Values_Spec.enso
Outdated
Show resolved
Hide resolved
distribution/lib/Standard/Table/0.0.0-dev/src/Internal/Table_Helpers.enso
Show resolved
Hide resolved
col_aggregate = if when_any then Aggregate_Column.Maximum _ else Aggregate_Column.Minimum _ | ||
col_aggregate = case when of | ||
Blank_Selector.Any_Cell_Blank -> Aggregate_Column.Maximum | ||
Blank_Selector.All_Cells_Blank -> Aggregate_Column.Minimum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work without the _
underscores??
IIRC without the underscore this uses default values and creates a Aggregate_Column.Maximum 0
which will always check the first column. That will inevitably be incorrect once columns.length > 1
.
Do the tests still succeed with that? If yes, I think we will need to expand the tests to ensure such an error is caught to avoid regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shifted this to include _ underscores
a2ba764
to
878823a
Compare
Added Blank_Selector constructor and applied to remove_blank_columns, select_blank_columns, filter_blank_rows
…tor.enso Co-authored-by: James Dunkerley <jdunkerley@users.noreply.github.com>
Co-authored-by: James Dunkerley <jdunkerley@users.noreply.github.com>
Added Blank_Selector constructor and applied to remove_blank_columns, select_blank_columns, filter_blank_rows
253362a
to
e415a95
Compare
There were errors before as these weren't updated
@@ -196,14 +197,15 @@ type Table_Column_Helper | |||
completely blank or have some blanks. | |||
|
|||
Arguments: | |||
- when_any: By default, only columns consisting of all blank cells are | |||
selected. If set to `True`, columns with one or more blank values are | |||
-TODO docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO
should be removed
@@ -0,0 +1,6 @@ | |||
## TODO Documents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should add a piece of docs here.
Pull Request Description
Added Blank_Selector constructor and applied to remove_blank_columns, select_blank_columns, filter_blank_rows for #7931 . Changed when_any to when for readability.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR: